implement pod exec websockets v5 by aojea · Pull Request #2486 · kubernetes-client/python

aojea · 2025-12-14T22:12:55Z

/kind feature
/kind api-change
/kind deprecation

support new exec v5 websocket subprotocol

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/4006-transition-spdy-to-websockets#proposal-new-remotecommand-sub-protocol-version---v5channelk8sio

cc: @yliaog @siyuanfoundation

k8s-ci-robot · 2025-12-14T22:13:04Z

Welcome @aojea!

It looks like this is your first PR to kubernetes-client/python 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/python has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-triage-robot · 2026-03-14T22:39:32Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2026-04-13T23:11:48Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

seans3 · 2026-04-23T17:17:46Z

/remove-lifecycle rotten

seans3 · 2026-04-23T17:18:22Z

/assign

seans3 · 2026-05-06T03:25:04Z

/lifecycle frozen

k8s-ci-robot · 2026-05-06T03:25:06Z

@seans3: The lifecycle/frozen label cannot be applied to Pull Requests.

Details

In response to this:

/lifecycle frozen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

seans3

This looks good and substantially complete.

One high-level observation: I believe falling back to v4.channel.k8s.io is the right approach for backwards compatibility, but we should probably note that for clusters < v1.30, the client will silently revert to the broken behavior and hang on EOF.

I've left a few minor inline comments mostly about tests and constants.

seans3 · 2026-05-06T03:40:00Z

                        del self._channels[channel]
                    return ret
+
+            if channel in self._closed_channels:


Do we need this same short-circuiting code in peek_channel and read_channel ?

seans3 · 2026-05-06T03:41:59Z

+        """Close a channel (v5 protocol only)."""
+        if self.subprotocol != V5_CHANNEL_PROTOCOL:
+            return
+        data = bytes([255, channel])


Should we define a constant (e.g. CLOSE_CHANNEL = 255 or V5_HALF_CLOSE = 255) for this magic number, and reference the constant?

+1 to have a constant

seans3 · 2026-05-06T03:45:04Z

+
+            mock_ws.send.assert_not_called()
+
+    def test_update_receives_close_v5(self):


Should we add an additional unit test to verify how readline_channel handles a closed channel with leftover data?

While the current unit tests cover the parsing of the close signal itself, the new logic you added inside readline_channel—which flushes the remaining buffer even if it lacks a newline—is currently untested. Having a test that asserts readline_channel successfully flushes leftover data (e.g. "hello") and then returns an empty string on the subsequent call would ensure this specific edge-case logic is protected from future regressions.

added tests also for read_channel and peek_channel to validate channels are drained and follow the expected semantics

seans3 · 2026-05-06T17:22:31Z

/assign @yliaog

yliaog · 2026-05-08T21:49:49Z

+                    # In Py3, iterating bytes gives int, but indexing bytes gives int.
+                    # websocket-client frame.data might be bytes.
+
+                    if channel == 255 and self.subprotocol == V5_CHANNEL_PROTOCOL: # v5 CLOSE


better to replace 255 with a constant

implementation is able to signal a channel is closed to the other side. Clients are also able to drain the closed channels.

aojea · 2026-05-12T14:14:48Z

@seans3 @yliaog PTAL

seans3 · 2026-05-12T17:35:58Z

Looks great--thanks.

/lgtm

k8s-ci-robot · 2026-05-12T17:36:01Z

@seans3: changing LGTM is restricted to collaborators

Details

In response to this:

Looks great--thanks.

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

yliaog · 2026-05-12T21:39:51Z

+        if resp.subprotocol != "v5.channel.k8s.io":
+            resp.close()
+            api.delete_namespaced_pod(name=name, body={}, namespace='default')
+            self.skipTest("Skipping test: v5.channel.k8s.io subprotocol not negotiated")


the test is skipped in the e2e test flow: https://github.com/kubernetes-client/python/actions/runs/25740181177/job/75636044165

how to make it run?

kubernetes/e2e_test/test_client.py::TestClient::test_pod_exec_close_channel SKIPPED

we need to get a more recent version of kind, let me update that

it was not the version, the e2e tests were not initializing the subprotocol so it never matched ... now the client if is not preconfigured, uses the subprotocol obtained during the negotiation

aojea · 2026-05-12T22:21:15Z

@yliaog is it normal for the e2e tests to take so long? they take more than 3 hours, that sounds like a lot

aojea · 2026-05-12T23:08:58Z

6a83c7e

it runs in less than 2 minutes in my vm 6a83c7e , it seems there can be a deadlock on stop

yliaog · 2026-05-13T03:14:46Z

the e2e takes a long time in the informer tests, i have not had a chance to look into why it takes so long.

aojea · 2026-05-13T05:51:50Z

the e2e takes a long time in the informer tests, i have not had a chance to look into why it takes so long.

Fixed im the latest commit, there was deadlockimg when.closing, added a comment

yliaog · 2026-05-13T16:23:06Z

thanks for the fix @aojea

thanks for the review @seans3

/lgtm
/approve

k8s-ci-robot · 2026-05-13T16:23:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, yliaog

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [yliaog]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested a review from fabianvf December 14, 2025 22:12

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Dec 14, 2025

k8s-ci-robot requested a review from roycaihw December 14, 2025 22:12

k8s-ci-robot added kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 14, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 14, 2025

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 14, 2026

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 13, 2026

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Apr 23, 2026

k8s-ci-robot assigned seans3 Apr 23, 2026

seans3 suggested changes May 6, 2026

View reviewed changes

k8s-ci-robot assigned yliaog May 6, 2026

yliaog reviewed May 8, 2026

View reviewed changes

implement pod exec v5

e557dec

implementation is able to signal a channel is closed to the other side. Clients are also able to drain the closed channels.

aojea force-pushed the exec_v5 branch from e177256 to e557dec Compare May 12, 2026 14:13

aojea changed the title ~~[WIP] implement pod exec v5~~ implement pod exec websockets v5 May 12, 2026

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 12, 2026

yliaog reviewed May 12, 2026

View reviewed changes

aojea added 2 commits May 12, 2026 22:05

use latest stable kind version

17c7610

use subprotocols from handshake

9b28fde

fix(watch): shutdown raw socket in stop() to prevent SSL read deadlocks

4846280

aojea force-pushed the exec_v5 branch from 6a83c7e to 4846280 Compare May 12, 2026 23:15

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2026

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2026

k8s-ci-robot merged commit 50d9ffb into kubernetes-client:master May 13, 2026
10 checks passed


		mock_ws.send.assert_not_called()

		def test_update_receives_close_v5(self):

Conversation

aojea commented Dec 14, 2025

Uh oh!

k8s-ci-robot commented Dec 14, 2025

Uh oh!

k8s-triage-robot commented Mar 14, 2026

Uh oh!

k8s-triage-robot commented Apr 13, 2026

Uh oh!

seans3 commented Apr 23, 2026

Uh oh!

seans3 commented Apr 23, 2026

Uh oh!

seans3 commented May 6, 2026

Uh oh!

k8s-ci-robot commented May 6, 2026

Uh oh!

seans3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

seans3 commented May 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea commented May 12, 2026

Uh oh!

seans3 commented May 12, 2026

Uh oh!

k8s-ci-robot commented May 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea commented May 12, 2026

Uh oh!

aojea commented May 12, 2026

Uh oh!

yliaog commented May 13, 2026

Uh oh!

aojea commented May 13, 2026

Uh oh!

yliaog commented May 13, 2026

Uh oh!

k8s-ci-robot commented May 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants